Skip to content

Add setInterrupt method to HardwareSerial class #3376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add setInterrupt method to HardwareSerial class #3376

wants to merge 2 commits into from

Conversation

ale-trevizoli
Copy link
Contributor

These changes allow to add an interrupt event for Serial using an method setInterrupt. Simple and Easy!

This is a resume of patch:
esp32-hal-uart.h
change function to allow one second parameter
void uartEnableInterrupt(uart_t* uart,void * func );
.
.
.
.
#ifdef __cplusplus
}
#endif

void uartDisableInterrupt(uart_t* uart);
void uartEnableInterrupt(uart_t* uart,void * func );

#endif /* MAIN_ESP32_HAL_UART_H_ */

esp32-hal-uart.h
changes on function _uart_isr
and
uartEnableInterrupt(uart_t* uart,void * func );
static void IRAM_ATTR _uart_isr(void arg)
{
uint8_t i, c;
BaseType_t xHigherPriorityTaskWoken;
uart_t uart;

if(arg != NULL) {
(*((void(**)())arg))();
}

for(i=0;i<3;i++){
uart = &_uart_bus_array[i];
if(uart->intr_handle == NULL){
continue;
}
uart->dev->int_clr.rxfifo_full = 1;
uart->dev->int_clr.frm_err = 1;
uart->dev->int_clr.rxfifo_tout = 1;
while(uart->dev->status.rxfifo_cnt || (uart->dev->mem_rx_status.wr_addr != uart->dev->mem_rx_status.rd_addr)) {
c = uart->dev->fifo.rw_byte;
if(uart->queue != NULL && !xQueueIsQueueFullFromISR(uart->queue)) {
xQueueSendFromISR(uart->queue, &c, &xHigherPriorityTaskWoken);
}
}
}

if (xHigherPriorityTaskWoken) {
portYIELD_FROM_ISR();
}
}

void uartEnableInterrupt(uart_t* uart,void * func)
{
UART_MUTEX_LOCK();
uart->dev->conf1.rxfifo_full_thrhd = 112;
uart->dev->conf1.rx_tout_thrhd = 2;
uart->dev->conf1.rx_tout_en = 1;
uart->dev->int_ena.rxfifo_full = 1;
uart->dev->int_ena.frm_err = 1;
uart->dev->int_ena.rxfifo_tout = 1;
uart->dev->int_clr.val = 0xffffffff;

esp_intr_alloc(UART_INTR_SOURCE(uart->num), (int)ESP_INTR_FLAG_IRAM, _uart_isr, func, &uart->intr_handle);
UART_MUTEX_UNLOCK();
}

HardwareSerial.h
add public function
void setInterrupt(void (*arg)() );
and
protected
void (*intr)() ;
class HardwareSerial: public Stream
{
public:
HardwareSerial(int uart_nr);
void setInterrupt(void (*arg)() );
void begin(unsigned long baud, uint32_t config=SERIAL_8N1, int8_t rxPin=-1, int8_t txPin=-1, bool invert=false, unsigned long timeout_ms = 20000UL);
void end();
void updateBaudRate(unsigned long baud);
int available(void);
int availableForWrite(void);
int peek(void);
int read(void);
void flush(void);
size_t write(uint8_t);
size_t write(const uint8_t *buffer, size_t size);

inline size_t write(const char * s)
{
return write((uint8_t*) s, strlen(s));
}
inline size_t write(unsigned long n)
{
return write((uint8_t) n);
}
inline size_t write(long n)
{
return write((uint8_t) n);
}
inline size_t write(unsigned int n)
{
return write((uint8_t) n);
}
inline size_t write(int n)
{
return write((uint8_t) n);
}
uint32_t baudRate();
operator bool() const;

size_t setRxBufferSize(size_t);
void setDebugOutput(bool);
protected:
void (intr)() ;
int _uart_nr;
uart_t _uart;
};

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
extern HardwareSerial Serial;
extern HardwareSerial Serial1;
extern HardwareSerial Serial2;
#endif

#endif

HardwareSerial.c
add function
void setInterrupt(void (*arg)() );
void HardwareSerial::setInterrupt(void(*arg)() )
{
uartDisableInterrupt(_uart);
intr = arg;
uartEnableInterrupt(_uart,&intr);
}

FINISH!!!
Example of USE

  Serial1.begin(9600,SERIAL_8N1, 17, 16); //RX,TX
  Serial1.setInterrupt(&SerialEvent1);

void IRAM_ATTR SerialEvent1(){ // ocorre quando o arduino recebe uma transmissão no RX1
while (Serial1.available()>0) {
Serial.print(Serial1.read());
}
Serial.println();
}

These changes allow to add an interrupt event for Serial using an method setInterrupt. Simple and Easy!

This is a resume of patch:
esp32-hal-uart.h
change function to allow one second parameter
void uartEnableInterrupt(uart_t* uart,void * func );
.
.
.
.
#ifdef __cplusplus
}
#endif

void uartDisableInterrupt(uart_t* uart);
void uartEnableInterrupt(uart_t* uart,void * func );

#endif /* MAIN_ESP32_HAL_UART_H_ */

esp32-hal-uart.h
changes on function _uart_isr
and
uartEnableInterrupt(uart_t* uart,void * func );
static void IRAM_ATTR _uart_isr(void arg)
{
uint8_t i, c;
BaseType_t xHigherPriorityTaskWoken;
uart_t uart;

if(arg != NULL) {
  (*((void(**)())arg))();
}

for(i=0;i<3;i++){
    uart = &_uart_bus_array[i];
    if(uart->intr_handle == NULL){
        continue;
    }
    uart->dev->int_clr.rxfifo_full = 1;
    uart->dev->int_clr.frm_err = 1;
    uart->dev->int_clr.rxfifo_tout = 1;
    while(uart->dev->status.rxfifo_cnt || (uart->dev->mem_rx_status.wr_addr != uart->dev->mem_rx_status.rd_addr)) {
        c = uart->dev->fifo.rw_byte;
        if(uart->queue != NULL && !xQueueIsQueueFullFromISR(uart->queue)) {
            xQueueSendFromISR(uart->queue, &c, &xHigherPriorityTaskWoken);
        }
    }
}

if (xHigherPriorityTaskWoken) {
    portYIELD_FROM_ISR();
}
}

void uartEnableInterrupt(uart_t* uart,void * func)
{
UART_MUTEX_LOCK();
uart->dev->conf1.rxfifo_full_thrhd = 112;
uart->dev->conf1.rx_tout_thrhd = 2;
uart->dev->conf1.rx_tout_en = 1;
uart->dev->int_ena.rxfifo_full = 1;
uart->dev->int_ena.frm_err = 1;
uart->dev->int_ena.rxfifo_tout = 1;
uart->dev->int_clr.val = 0xffffffff;

esp_intr_alloc(UART_INTR_SOURCE(uart->num), (int)ESP_INTR_FLAG_IRAM, _uart_isr, func, &uart->intr_handle);
UART_MUTEX_UNLOCK();
}

HardwareSerial.h
add public function
void setInterrupt(void (*arg)() );
and
protected
void (*intr)() ;
class HardwareSerial: public Stream
{
public:
HardwareSerial(int uart_nr);
void setInterrupt(void (*arg)() );
void begin(unsigned long baud, uint32_t config=SERIAL_8N1, int8_t rxPin=-1, int8_t txPin=-1, bool invert=false, unsigned long timeout_ms = 20000UL);
void end();
void updateBaudRate(unsigned long baud);
int available(void);
int availableForWrite(void);
int peek(void);
int read(void);
void flush(void);
size_t write(uint8_t);
size_t write(const uint8_t *buffer, size_t size);

inline size_t write(const char * s)
{
    return write((uint8_t*) s, strlen(s));
}
inline size_t write(unsigned long n)
{
    return write((uint8_t) n);
}
inline size_t write(long n)
{
    return write((uint8_t) n);
}
inline size_t write(unsigned int n)
{
    return write((uint8_t) n);
}
inline size_t write(int n)
{
    return write((uint8_t) n);
}
uint32_t baudRate();
operator bool() const;

size_t setRxBufferSize(size_t);
void setDebugOutput(bool);
protected:
void (intr)() ;
int _uart_nr;
uart_t _uart;
};

#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
extern HardwareSerial Serial;
extern HardwareSerial Serial1;
extern HardwareSerial Serial2;
#endif

#endif

HardwareSerial.c
add function
void setInterrupt(void (*arg)() );
void HardwareSerial::setInterrupt(void(*arg)() )
{
uartDisableInterrupt(_uart);
intr = arg;
uartEnableInterrupt(_uart,&intr);
}

FINISH!!!
Example of USE

      Serial1.begin(9600,SERIAL_8N1, 17, 16); //RX,TX
      Serial1.setInterrupt(&SerialEvent1);
void IRAM_ATTR SerialEvent1(){ // ocorre quando o arduino recebe uma transmissão no RX1
while (Serial1.available()>0) {
Serial.print(Serial1.read());
}
Serial.println();
}
@ale-trevizoli
Copy link
Contributor Author

@stickbreaker
Copy link
Contributor

stickbreaker commented Oct 17, 2019

Problem with adding this code, the current UART ISR services all three UARTS on any interrupt. So, if you register this interrupt callback for Serial() It will also trigger for any received characters on Serial1() or Serial2().

Edit for a better explanation of the problem.
The current ISR services all UARTS whenever one of them generates an Interrupt. This callback will only trigger if the Interrupt is generated by the "attached" UART. So if UART1 has the callback attached, but UART0 triggers the interrupt, it will clear UART1's fifo, so, UART1 will not ever generate an interrupt.

Chuck.

int _uart_nr;
uart_t* _uart;
};

extern void serialEventRun(void) __attribute__((weak));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be restored as this weakly declared method is called, this also breaks the tests which prevent this PR from being considered for merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was taken care of in #2991 prior to this :-)

}

// void uartAttachRx(uart_t* uart, uint8_t rxPin, bool inverted)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this method commented out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, why?

@me-no-dev
Copy link
Member

please do not forget to properly format the description of this pull request. What you have posted above is unreadable.

@@ -96,6 +100,22 @@ static void IRAM_ATTR _uart_isr(void *arg)
}
}

void uartEnableInterrupt(uart_t* uart,void * func)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why didn't you just change the original function?

if(arg != NULL) {
(*((void(**)())arg))();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check should be made in the while loop below and byte c should be sent to the callback instead of to the queue. If callback is not in IRAM, things will go really bad really fast.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@me-no-dev I don't think this could be used as an input filter. As written, it triggers once per interrupt, but, the interrupt can empty up to "fifo" of chars. see my comment above.

Corrected all Find, FindUntil and added findMulti function, copy code from Stream.h and Stream.cpp from AVR-CORE.
Stream.find was not working when used with Ethernet.find()
After copy code from AVR-CORE it is working now
@ianr34
Copy link

ianr34 commented Jan 8, 2020

I agree this patch needs to be passing the tests - but I think some of the posters above don't get the real need for this (it should have been in one of the very early releases!).
It doesn't matter what serial port has had input. It doesn't matter having it go per character, or not. It just needs to exist so that we don't have to do a tight loop continuing to poll the serial port - a complete waste of resources!
You just want to have an interrupt that will set an event, which will wake up a thread that can THEN see which serial port it was and then read the data...

@AlessandroAU
Copy link

Any updates on this? would really like to see this merged. As @ianr34 said it's a fundamental missing feature in the hardware serial class.

@ianr34
Copy link

ianr34 commented Apr 15, 2020

yep, no answer and it isn't in. I must admin that I've been moving all my code esp32 away from the ardunio-esp32 - you have to go edit them too many times to fix or change the behaviour of things.
I'm not quite done yet - I've got rid of all the libraries (as many of them seemed to be ported from the 8266 and not redone for the esp32 to take it's better hardware into account) and am working to getting rid of the core too - the serial port and a few odds and ends are all that's left to go..

This issue is an example I've why I'm heading in that direction, it should have been fixed/ added in the first release of ardunio-esp32 as the serial port is pretty fundamental to what most of us do, and getting an interrupt to say there is data - I don't care what serial port is raising it (it the comments above) as I can then check - is a very good idea... Bluetooth gives you and interrupt. Wifi gives you and interrupt. Serial doesn't.....

@timkoers
Copy link
Contributor

Please checkout #4656

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@SuGlider
Copy link
Collaborator

Check PR #6134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants